Add new codemirror-promql-based expression editor #8634
Conversation
Looking at setting a higher Node version on Netlify... |
juliusv
force-pushed
the
feature/codemirror-editor
branch
from
4e40e4a
to
318bfbf
Compare
|
Note that on Netlify, the autocompletion of metric names currently does not work because I haven't hacked new magic into Netlify yet to bend API requests over to our Prometheus demo server (instead of going directly to Netlify). |
|
CircleCI's Node version is too old too (12 vs 14), but updating it is more cumbersome than on Netlify :/ https://circleci.com/docs/2.0/circleci-images/#best-practices Seems like we would have to add Node update commands to the |
|
I'm not sure yet though why the tests on CircleCI have passed, but the build step is failing on a too old node version. |
@simonpasquier @roidelapluie could someone more familiar with the CircleCI build system help figure out why that is? |
|
@Nexucis Yes, same as I fixed on Netlify already. But why does it only happen for the |
|
Comparing the "Spin up environment" steps on CircleCI,
...whereas |
|
A I think we should not default to the new editor, but make it default next release. I also don't think we should "hide" the checkboxes,instead I would disable them. I would also put them closer to the new editor checkbox, specifying that it is experimental. Looking into the build failure now. |
|
@roidelapluie I'll get right on that if you are looking at the build stuff :) |
| options: this.queryHistory.map(q => ({ | ||
| label: q.length < 80 ? q : q.slice(0, 76).concat('...'), | ||
| detail: 'past query', | ||
| apply: q, |
There was a problem hiding this comment.
perhaps for this case, you should have a more sofisticated apply method. It's only in case you would like to replace the whole current expression when you are autocompleting a query from the history.
There was a problem hiding this comment.
I think I'm happy enough with how it currently works: it only inserts at position 1, but doesn't overwrite anything that is already there... maybe it makes sense to play with it and optimize it in the future, if someone has a better idea.
There was a problem hiding this comment.
alright :). Fine for me too, I was just suggesting since I wasn't sure about the expected behavior expected of the query history
|
|
||
| const { state, pos } = context; | ||
| const tree = syntaxTree(state).resolve(pos, -1); | ||
| const start = computeStartCompletePosition(tree, pos); |
There was a problem hiding this comment.
I think it's not a really good idea to use this method. You are quite lucky it is exported after all and it is not really part of what the library would like to expose. It's more a side effect here.
Perhaps something like that could work too :
return Promise.resolve(this.complete.promQL(context)).then( res => {
const { state, pos } = context;
const tree = syntaxTree(state).resolve(pos, -1);
const start = res != null ? res.from : tree.from
if (start !== 0) {
return res;
}
// then put the logic to add the historyItems here
});There was a problem hiding this comment.
Thanks, that looks good! Changed it to that.
| label: q.length < 80 ? q : q.slice(0, 76).concat('...'), | ||
| detail: 'past query', | ||
| apply: q, | ||
| info: q, |
There was a problem hiding this comment.
perhaps add only q if it has been truncated ? And nothing otherwise
| apply: q, | ||
| info: q, | ||
| })), | ||
| span: /^[a-zA-Z0-9_:]+$/, |
There was a problem hiding this comment.
not sure it is really important here, but there are some cases where the span is not added. I think the case appears when you are trying to autocomplete a duration.
So maybe we don't really care about that in this particular case
There was a problem hiding this comment.
Yeah, I wasn't sure about that either. But if I remove the span here completely, then the completion dropdown disappears and reappears on each typed character (which is slow and ugly), because the search is starting completely from zero.
There was a problem hiding this comment.
maybe you can do that then:
span: res !== nul ? res.span : /^[a-zA-Z0-9_:]+$/,Like that you won't erase what is coming from the previous complete object
There was a problem hiding this comment.
or let's keep it like you did. And let's see if there are some weird cases ^^. I don't really think someone will be able to reach this case anyway. So should be good
|
@roidelapluie grouped the new boxes, renamed "new editor" to "experimental editor", and defaulting to the old editor for now. Take another look :) |
|
Thanks! We should probably disable query history and autocomplete when new editor is on. |
@roidelapluie Hm, why? Both are applicable to the new editor as well. Without autocomplete, it would be pretty sad in fact :) |
didn't notice they work on both. |
|
@Nexucis Thanks, will add "Fixes: ..." lines! The last one has already been fixed by the way, there's already a metrics explorer now. |
Signed-off-by: Julius Volz <julius.volz@gmail.com>
Signed-off-by: Julius Volz <julius.volz@gmail.com>
Signed-off-by: Julius Volz <julius.volz@gmail.com>
Signed-off-by: Julius Volz <julius.volz@gmail.com>
Signed-off-by: Julius Volz <julius.volz@gmail.com>
Signed-off-by: Julius Volz <julius.volz@gmail.com>
Signed-off-by: Julius Volz <julius.volz@gmail.com>
Signed-off-by: Julius Volz <julius.volz@gmail.com>
juliusv
force-pushed
the
feature/codemirror-editor
branch
from
67f8e3a
to
4f112cc
Compare
|
@roidelapluie Good to merge now? |
|
@juliusv this is seriously amazing work, it will make learning PromQL much easier now, excellent job! |
|
@flyinprogrammer Thank you (also to @Nexucis, who built a lot of the CodeMirror PromQL integration code). |
|
thanks @flyinprogrammer :) |
This adds advanced autocompletion, syntax highlighting, and linting
for PromQL.
Signed-off-by: Julius Volz julius.volz@gmail.com